Always call launchUrlVSCode, error or not #6780
Conversation
This should prevent a repeat of flutter#6105 if an exception occurs while trying to open the page when embedded inside VS Code.
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/devtools/6780, due to - The status or check suite Verify PR Release Note Requirements has failed. Please fix the issues identified (or deflake) before re-applying this label. |
It has not, it was green when you checked! 😄 |
| try { | ||
| if (parsedUrl != null && await url_launcher.canLaunchUrl(parsedUrl)) { | ||
| await url_launcher.launchUrl(parsedUrl); | ||
| } else { | ||
| notificationService.push('Unable to open $url.'); | ||
| } | ||
| } finally { | ||
| // Always pass the request up to VS Code because we could fail both silently | ||
| // (the usual behaviour) or with another error like | ||
| // "Attempted to call Window.open with a null window" | ||
| // https://github.com/flutter/devtools/issues/6105. | ||
| // | ||
| // In the case where we are not in VS Code, there will be nobody listening | ||
| // to the postMessage this sends. | ||
| launchUrlVSCode(url); | ||
| } |
There was a problem hiding this comment.
would it be better to just call launchUrlVSCode by default when ideTheme.embed == true? instead of attempting to call something we expect to fail?
There was a problem hiding this comment.
for example, what if at some point, url_launcher.launchUrl starts working for the embedded environment? then we'd be launching the url twice.
There was a problem hiding this comment.
We don't expect launchUrlVSCode to fail, it will just silently do nothing (it'll postMessage but nobody is listening).
We can check that though - is there any possibility of ending up here without it being on globals? (I don't think so, but it feels slightly riskier than the change to just call this when an exception occurs for a cherry-pick)
There was a problem hiding this comment.
Maybe my comment "we could fail" above is a bit misleading - it's referring to the existing url_launcher code.
There was a problem hiding this comment.
Okay, yeah since we were already always calling launchUrlVSCode, then putting this in finally is probably safer. So the real change here is just wrapping in a try block to swallow the exception.
There was a problem hiding this comment.
Just to be clear - the launchUrlVSCode method does not throw, the issue was that url_launcher.launchUrl would sometimes throw (in the previous version, not the current stable).
This code was originally written to always call launchUrlVSCode, but in the case where url_launcher.launchUrl throws (something we never expected but was happening in the previous stable), it would be skipped.
My change here just moves launchUrlVSCode into a finally block so that it will be called also if url_launcher.launchUrl throws, and not only if it completed without exception.
I didn't use a catch because I didn't want to mask the original (unexpected) exception if it comes back, but this way we'll at least still open the URL from VS Code instead of silently doing nothing.
auto-submit
bot
removed
the
autosubmit
|
auto label is removed for flutter/devtools/6780, due to - The status or check suite Verify PR Release Note Requirements has failed. Please fix the issues identified (or deflake) before re-applying this label. |
This should prevent a repeat of #6105 if an exception occurs while trying to open the page when embedded inside VS Code.
This should prevent a repeat of #6105 if an exception occurs while trying to open the page when embedded inside VS Code.